Nip46 auth with NDK #1636
Nip46 auth with NDK #1636
Conversation
|
No dependency changes detected. Learn more about Socket for GitHub ↗︎ 👍 No dependency changes detected in pull request |
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
Co-authored-by: Copilot <[email protected]>
Co-authored-by: ekzyis <[email protected]>
|
I went ahead and rebased this branch on #1590, I hope you don't mind. However, I wonder if this is in draft because it depends on #1590 even though #1590 itself was ready for review? Or is this in draft because the changes between this PR and #1590 ( Also, can you use the PR template which mentions how you tested this (which remote signer etc.) or if there was anything unclear during implementation? The template helps a lot with reviews. |
components/nostr-auth.js
Outdated
| <p> | ||
| Please confirm this action on your remote signer. | ||
| </p> | ||
| {!challengeUrl && (<pre>{challenge}</pre>)} |
There was a problem hiding this comment.
nip46 doesn't say this must always be an url, so i've added some extra code to show it as a text if it is not a valid url.
It could contain instruction like "open your signer and confirm"
| console.log('nostr auth error', e) | ||
| setExtensionError({ message: `${text} failed`, details: e.message }) | ||
| // authorize user | ||
| const auth = useCallback(async (nip46token) => { |
There was a problem hiding this comment.
Since NDK has this nice signer abstraction, i've made this single callback to handle both nip46 and nip07 logins.
components/nostr-auth.js
Outdated
| </li> | ||
| <li> | ||
| <a href='https://app.nsecbunker.com/'>nsecBunker</a><br /> | ||
| available as: SaaS or self-hosted |
There was a problem hiding this comment.
the original PR had more signers in this list, but i've found they have issues or don't seem to be very well maintained, so i've decided to keep only these two
Sorry, I meant two confirmations in the signer required. I think the solution would be to use NIP-98 since permission to sign
Even though my idea about running the signer on the server is no longer relevant thanks to NIP-98, which user secrets would this expose since this is remote signing? Do you mean the secrets in the bunker URL?
Ah okay, makes sense. I forgot I was using firefox where I didn't have the nostr extension so I thought this broke crossposting but it didn't.
Oh yes, that was the issue. Works with fresh tokens. Is it possible to handle this? Only showing a spinner with no info is bad UX (related to my question why you decided to use a modal). |
should be fixed now, i've added the same workaround
i've replaced the modal with this
|
you mean the two popups that comes up in nsec.app?
yes the server could steal the bunker secret NIP-98 seems another auth method that is unrelated to NIP-46
we can only add an arbitrary timeout, it is the signer (nsec.app) that decides to ignore these events instead of replying with an error |
i think it's ndk, see return new Promise((resolve, reject) => {
const connectParams = [this.userPubkey ?? ""];
if (this.secret) connectParams.push(this.secret);
if (!this.bunkerPubkey) throw new Error("Bunker pubkey not set");
this.rpc.sendRequest(
this.bunkerPubkey,
"connect",
connectParams,
24133,
(response) => {
if (response.result === "ack") {
this.getPublicKey().then((pubkey) => {
this._user = new NDKUser({ pubkey });
resolve(this._user);
});
} else {
reject(response.error);
}
}
);
});it should include a third |
yes
I think it's an issue with how we're using NIP-46 for our authentication. We don't need to ask for additional permissions, we're just not using the permissions we're given:
NIP-46 is only remote signing, it's not actually an auth method. NIP-98 is the auth method. It's the same that we currently do (sign an event) but standardized so that we don't have to ask for permission to sign the event since that permission was already granted
We could show a message like "are you sure you used a fresh token?" after the timeout but not actually abort the login because the user might just be slow with pressing "confirm" in their signer |
|
Ah, i get what you mean now. Is it defined anywhere that kind 27235 is supposed to be a default permissions or is it a choice of nsec.app? EDIT: changed to use kind 27235 |
I don't know, but it's at least a standard.
Thanks, much better now! |
NIP-98 requires its own flow see. It is basically nostr basic auth, i don't think it is worth (or even possible) to implement all that for us, so i've just added some generic metadata to make it "pass" just in case some signer is doing some extra validation |
There was a problem hiding this comment.
Some CSS suggestions:
diff --git a/components/nostr-auth.js b/components/nostr-auth.js
index 697853af..e056516b 100644
--- a/components/nostr-auth.js
+++ b/components/nostr-auth.js
@@ -138,13 +138,13 @@ export function NostrAuth ({ text, callbackUrl, multiAuth }) {
return (
<>
- <h3 className='w-100 pb-2'>{status.title ? status.title : ((text || 'Login') + ' with Nostr')}</h3>
+ <h3 className='w-100 mb-0 pb-3 text-center'>{status.title ? status.title : ((text || 'Login') + ' with Nostr')}</h3>
{status.error && <NostrError message={status.msg} />}
{status.loading
? (
<>
- <Moon className='spin fill-grey' width='50' height='50' />
- <div className='text-muted pt-4 pb-4 w-100'>{status.msg}</div>
+ <Moon className='spin fill-grey' width='24' height='24' />
+ <div className='text-muted py-3 w-100 text-center'>{status.msg}</div>
{status.button && (
<Button
className='w-100' variant='primary'
before
after
How does the NIP-05 login work and how can I test it? I see that NIP-46 mentions this now:
nip05 login is removed
Should we drop it and only mention bunker://?
|
Added the timeout
|
wouldn't this make it look a bit odd compared to the other login screens and the general sn layout that is generally aligned to left?
|
you can use this from the same nsec.app page |
Co-authored-by: ekzyis <[email protected]>
There was a problem hiding this comment.
wouldn't this make it look a bit odd compared to the other login screens and the general sn layout that is generally aligned to left?
Mhh, I don't know. I just felt that the spinner in the center with everything else not in the center looks odd. But maybe it's just me or it's just too big for me.
So please ignore if you don't agree, @huumn will have the last word anyway.
Going to approve this now since my comment about the timeout implementation and the wording of the hint are mostly just nitpicks.
| const [suggestion, setSuggestion] = useState(null) | ||
| const suggestionTimeout = useRef(null) |
There was a problem hiding this comment.
Sorry for the micromanagement, but do you really need 1x useState, 1x useRef, 1x useEffect and two functions to show a message after some delay? Is this not enough:
diff --git a/components/nostr-auth.js b/components/nostr-auth.js
index 9cc85716..7ea093db 100644
--- a/components/nostr-auth.js
+++ b/components/nostr-auth.js
@@ -42,8 +42,7 @@ export function NostrAuth ({ text, callbackUrl, multiAuth }) {
button: undefined
})
- const [suggestion, setSuggestion] = useState(null)
- const suggestionTimeout = useRef(null)
+ const [showSuggestion, setShowSuggestion] = useState(false)
const toaster = useToast()
const challengeResolver = useCallback(async (challenge) => {
@@ -93,23 +92,6 @@ export function NostrAuth ({ text, callbackUrl, multiAuth }) {
})
}, [])
- const clearSuggestionTimer = () => {
- if (suggestionTimeout.current) clearTimeout(suggestionTimeout.current)
- }
-
- const setSuggestionWithTimer = (msg) => {
- clearSuggestionTimer()
- suggestionTimeout.current = setTimeout(() => {
- setSuggestion(msg)
- }, 10_000)
- }
-
- useEffect(() => {
- return () => {
- clearSuggestionTimer()
- }
- }, [])
-
// authorize user
const auth = useCallback(async (nip46token) => {
setStatus({
@@ -132,9 +114,9 @@ export function NostrAuth ({ text, callbackUrl, multiAuth }) {
signer.once('authUrl', challengeResolver)
}
- setSuggestionWithTimer('Having trouble? Try to use a fresh token or NIP-05 address')
+ const timeout = setTimeout(() => setShowSuggestion(true), 10_000)
await signer.blockUntilReady()
- clearSuggestionTimer()
+ clearTimeout(timeout)
setStatus({
msg: 'Signing in',
@@ -160,8 +142,6 @@ export function NostrAuth ({ text, callbackUrl, multiAuth }) {
})
} catch (e) {
setError(e)
- } finally {
- clearSuggestionTimer()
}
}, [])
@@ -182,8 +162,10 @@ export function NostrAuth ({ text, callbackUrl, multiAuth }) {
{status.button.label}
</Button>
)}
- {suggestion && (
- <div className='text-muted text-center small pt-2'>{suggestion}</div>
+ {showSuggestion && (
+ <div className='text-muted text-center small pt-2'>
+ Having trouble? Make sure you used a fresh token or a valid NIP-05 address.
+ </div>
)}
</>
)Did you do it this way so you can clear the timeout when the page unmounts? From what I can tell, this doesn't seem to be required but if so, I think only useRef and useEffect is what you need. The suggestion message doesn't require state imo. it can just be hardcoded. So probably related to what you mentioned in the other comment with your preference of avoiding to hardcode stuff.
There was a problem hiding this comment.
The clearSuggestionTimer in the finally block is to avoid causing an useless state update if we are showing the error, the clearSuggestionTimer on unmount is to not hold the context (up to 10 seconds) from the garbage collector. Generally i find it is a good practice to make code that clean after itself.
Co-authored-by: ekzyis <[email protected]>
️✅ There are no secrets present in this pull request anymore.If these secrets were true positive and are still valid, we highly recommend you to revoke them. 🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request. |
Description
Nip46 auth using NDK (reimplementation of #1311)
Depends on #1590 closes #864
This PR updates the behavior of the "Login with Nostr" button by adding an intermediary page that presents the following login options:
Screenshots
Additional Context
The login by qr code/nostrconnect:// has been removed due to the thing we discussed.
I've been testing using https://nsec.app/
Checklist
Are your changes backwards compatible? Please answer below:
yes
Did you QA this? Could we deploy this straight to production? Please answer below:
yes , tested both bunker:// , nip-05 and extension to ensure there is no regression.
For frontend changes: Tested on mobile? Please answer below:
Yes
Did you introduce any new environment variables? If so, call them out explicitly here:
no